Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn screen off when closing scrcpy #824

Closed
wants to merge 8 commits into from

Conversation

npes87184
Copy link
Contributor

Fix #714

Copy link
Collaborator

@rom1v rom1v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR.

I think this should be an option: I often plug my device just to type or copy-paste things from the computer, I don't want it to be turned off just because I close scrcpy.

@@ -25,6 +25,7 @@ enum control_msg_type {
CONTROL_MSG_TYPE_GET_CLIPBOARD,
CONTROL_MSG_TYPE_SET_CLIPBOARD,
CONTROL_MSG_TYPE_SET_SCREEN_POWER_MODE,
CONTROL_MSG_TYPE_TURN_SCREEN_OFF,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to distinguish between "turn screen off but keep device on" (like here) and "turn screen off and device off (standby)". (There is already some naming confusion in the current codebase, if you have any suggestion…)

Copy link
Contributor Author

@npes87184 npes87184 Sep 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "lock screen" vs "turn screen off"?

@rom1v
Copy link
Collaborator

rom1v commented Sep 28, 2019

I merged the first commit fixing the log 👍

@npes87184
Copy link
Contributor Author

I have pushed v2:

  • Let this feature be optional.
  • use "lock screen"

However, I found that server may be killed before screen off.
We may need to introduce some ack mechanism from server to client.

@rom1v
Copy link
Collaborator

rom1v commented Oct 31, 2019

However, I found that server may be killed before screen off.
We may need to introduce some ack mechanism from server to client.

In my TODO list, I plan to implement something similar to #679 to be able to cleanup properly:

The server "forks" itself for full life cycle control (adb may kill it instead preventing required cleanup).

This will be a requirement to merge this PR.

@rom1v
Copy link
Collaborator

rom1v commented Aug 3, 2020

Now that there is a real cleanup, you could implement it 😉

(I suggest --power-off-on-close)

@npes87184
Copy link
Contributor Author

Ok, I will look it.

@npes87184
Copy link
Contributor Author

npes87184 commented Feb 16, 2021

Hi, @rom1v

There is a problem about power off screen in CleanUp process. In this time, injectEvent will consider about displayId to inject to correct target. However, it is diffiuclt to allocate Device in the process of CleanUp. As an result, I cannot trigger power off screen in that stage. Do you have any good advice?

Maybe CleanUp process should have the same environment just likes the main server?

Thank you.

@rom1v
Copy link
Collaborator

rom1v commented Feb 16, 2021

As a quick-and-dirty solution, maybe you could expose a static injectEvent() taking a displayId:

diff --git a/server/src/main/java/com/genymobile/scrcpy/Device.java b/server/src/main/java/com/genymobile/scrcpy/Device.java
index a8fdf677..9fb365ff 100644
--- a/server/src/main/java/com/genymobile/scrcpy/Device.java
+++ b/server/src/main/java/com/genymobile/scrcpy/Device.java
@@ -157,16 +157,20 @@ public final class Device {
         return supportsInputEvents;
     }
 
+    public static boolean injectEvent(InputEvent inputEvent, int mode, int displayId) {
+        if (displayId != 0 && !InputManager.setDisplayId(inputEvent, displayId)) {
+            return false;
+        }
+
+        return SERVICE_MANAGER.getInputManager().injectInputEvent(inputEvent, mode);
+    }
+
     public boolean injectEvent(InputEvent inputEvent, int mode) {
         if (!supportsInputEvents()) {
             throw new AssertionError("Could not inject input event if !supportsInputEvents()");
         }
 
-        if (displayId != 0 && !InputManager.setDisplayId(inputEvent, displayId)) {
-            return false;
-        }
-
-        return SERVICE_MANAGER.getInputManager().injectInputEvent(inputEvent, mode);
+        return injectEvent(inputEvent, mode, displayId);
     }
 
     public boolean injectEvent(InputEvent event) {

Then, you have to transmit both the displayId and the flag to NOT turn off if control is disabled.

What do you think?

@npes87184
Copy link
Contributor Author

npes87184 commented Feb 17, 2021

As a quick-and-dirty solution, maybe you could expose a static injectEvent() taking a displayId:

diff --git a/server/src/main/java/com/genymobile/scrcpy/Device.java b/server/src/main/java/com/genymobile/scrcpy/Device.java
index a8fdf677..9fb365ff 100644
--- a/server/src/main/java/com/genymobile/scrcpy/Device.java
+++ b/server/src/main/java/com/genymobile/scrcpy/Device.java
@@ -157,16 +157,20 @@ public final class Device {
         return supportsInputEvents;
     }
 
+    public static boolean injectEvent(InputEvent inputEvent, int mode, int displayId) {
+        if (displayId != 0 && !InputManager.setDisplayId(inputEvent, displayId)) {
+            return false;
+        }
+
+        return SERVICE_MANAGER.getInputManager().injectInputEvent(inputEvent, mode);
+    }
+
     public boolean injectEvent(InputEvent inputEvent, int mode) {
         if (!supportsInputEvents()) {
             throw new AssertionError("Could not inject input event if !supportsInputEvents()");
         }
 
-        if (displayId != 0 && !InputManager.setDisplayId(inputEvent, displayId)) {
-            return false;
-        }
-
-        return SERVICE_MANAGER.getInputManager().injectInputEvent(inputEvent, mode);
+        return injectEvent(inputEvent, mode, displayId);
     }
 
     public boolean injectEvent(InputEvent event) {

Then, you have to transmit both the displayId and the flag to NOT turn off if control is disabled.

What do you think?

Oh, yes. This solution works. However, I am thinking about future. It will have more and more things that need to be done in CleanUp. If we need to pass variables likes this in each time, it is very annoy and dirty.

For infrastructure, in my opinion, in CleanUp process, I prefer to prepare a environment similar to the main process.
It just needs to handle the options correctly. Maybe it can add two methods, toFile and fromFile, to the class options.

What do you think?

After the struct screen is initialized, the window and the renderer are
necessarily valid, so there is no need o check in screen_destroy().
This paves the way to handle EVENT_NEW_FRAME from screen.c, by allowing
to call screen_update_frame() without an explicit video_buffer instance.
Now that all screen-related events are handled from screen.c, there is
no need for a separate method for window events.
@rom1v
Copy link
Collaborator

rom1v commented Feb 18, 2021

It just needs to handle the options correctly.

It could be great to serialize some structure between the main process and the cleanup process indeed.

However, this structure should not be the existing Options, which is neither necessary (not all fields are meaningful for the cleanup process) nor sufficient (the cleanup actions may for example depend on the initial state of the device, which is not stored in the options).

(I don't know if in the end such a structure would be helpful compared to passing each option as argument.)

But yes, it would be great to share the common part of the Device initialization between the main and cleanup process. For now, Device is quite a mess, and it's not clear semantically what this class represents.

@npes87184
Copy link
Contributor Author

Ok, I will pass displayId in this time. The refactoring of Device and the environment preparation of CleanUp should be another problems (or features).

Thank you for your help.

@npes87184 npes87184 changed the base branch from master to dev February 21, 2021 00:44
@npes87184
Copy link
Contributor Author

Hi, @rom1v

I have added two commits about this features.

First, I export a series static methods for powering off. Second, make CleanUp process respects the power off setting.

Thanks.

@rom1v
Copy link
Collaborator

rom1v commented Feb 21, 2021

Thank you very much for your work 👍

I just tested, and I found a problem (on a Nexus 5): the "power off" cleanup seems to interact badly with the "restore normal power mode" cleanup, which is executed whenever the screen is on on exit. As a result, --power-off-on-close works randomly (on this device). (EDIT: this may not be the problem: #824 (comment))

I tried with these changes:

diff --git a/server/src/main/java/com/genymobile/scrcpy/CleanUp.java b/server/src/main/java/com/genymobile/scrcpy/CleanUp.java
index 7dda83fb..26012409 100644
--- a/server/src/main/java/com/genymobile/scrcpy/CleanUp.java
+++ b/server/src/main/java/com/genymobile/scrcpy/CleanUp.java
@@ -84,16 +84,14 @@ public final class CleanUp {
             }
         }
 
-        if (restoreNormalPowerMode) {
-            Ln.i("Restoring normal power mode");
-            if (Device.isScreenOn()) {
+        if (Device.isScreenOn()) {
+            if (powerOffScreen) {
+                Ln.i("Power off screen");
+                Device.powerOffScreen(displayId);
+            } else if (restoreNormalPowerMode) {
+                Ln.i("Restoring normal power mode");
                 Device.setScreenPowerMode(Device.POWER_MODE_NORMAL);
             }
         }
-
-        if (powerOffScreen) {
-            Ln.i("Power off screen");
-            Device.powerOffScreen(displayId);
-        }
     }
 }

It improves the behavior.

But I have an additional issue that can I cannot explain: if I keep touching the device (I keep one finger on the device screen) while I close the scrcpy window, it blocks and never power off the screen. It is unexpected, because if I keep one finger on the screen and press MOD+p to power off the screen, then it works. 😕

If I add some logs (and even if I always return true) in Device.isScreenOn():

diff --git a/server/src/main/java/com/genymobile/scrcpy/CleanUp.java b/server/src/main/java/com/genymobile/scrcpy/CleanUp.java
index 26012409..2c54558c 100644
--- a/server/src/main/java/com/genymobile/scrcpy/CleanUp.java
+++ b/server/src/main/java/com/genymobile/scrcpy/CleanUp.java
@@ -84,7 +84,9 @@ public final class CleanUp {
             }
         }
 
+        Ln.i("CLEANUP #1");
         if (Device.isScreenOn()) {
+            Ln.i("CLEANUP #2");
             if (powerOffScreen) {
                 Ln.i("Power off screen");
                 Device.powerOffScreen(displayId);
@@ -93,5 +95,6 @@ public final class CleanUp {
                 Device.setScreenPowerMode(Device.POWER_MODE_NORMAL);
             }
         }
+        Ln.i("CLEANUP #3");
     }
 }
diff --git a/server/src/main/java/com/genymobile/scrcpy/Device.java b/server/src/main/java/com/genymobile/scrcpy/Device.java
index 624c9fa0..3ee248d3 100644
--- a/server/src/main/java/com/genymobile/scrcpy/Device.java
+++ b/server/src/main/java/com/genymobile/scrcpy/Device.java
@@ -212,7 +212,7 @@ public final class Device {
     }
 
     public static boolean isScreenOn() {
-        return SERVICE_MANAGER.getPowerManager().isScreenOn();
+        return true; //SERVICE_MANAGER.getPowerManager().isScreenOn();
     }
 
     public synchronized void setRotationListener(RotationListener rotationListener) {

Then it blocks:

02-21 11:24:23.251 21036 21056 D scrcpy  : Device message sender stopped
02-21 11:24:23.485 21054 21054 I scrcpy  : Cleaning up
02-21 11:24:23.485 21054 21054 I scrcpy  : CLEANUP #1

If I replace Device.isScreenOn() by true, then it prints CLEANUP #2, but and Power off screen, but does not power off the screen. 🤔

@rom1v
Copy link
Collaborator

rom1v commented Feb 21, 2021

In fact, it seems the cleanup process is killed (the process does not appear in ps).

Btw, if I add a SystemClock.sleep(1000); (even if I don't touch the device), the process is closed before CLEANUP #1.
The fact that I touch the screen is irrelevant (it probably just change the timings and makes it more probable).

It's probably due to the OMXNodeInstance: !!! Observer died:

02-21 11:45:31.420 22574 22593 D scrcpy  : Controller stopped
02-21 11:45:32.469 22592 22592 I scrcpy  : Cleaning up
02-21 11:45:32.478 28310 29411 E OMXNodeInstance: !!! Observer died. Quickly, do something, ... anything...
02-21 11:45:32.590 28310 29411 W GraphicBufferSource: Dropped back down to Loaded without Executing
02-21 11:45:32.624 28310 29411 I OMX-VENC: Component Deinit

(I don't have this problem on my OnePlus 7 Pro)

@npes87184
Copy link
Contributor Author

In fact, it seems the cleanup process is killed (the process does not appear in ps).

Btw, if I add a SystemClock.sleep(1000); (even if I don't touch the device), the process is closed before CLEANUP #1.
The fact that I touch the screen is irrelevant (it probably just change the timings and makes it more probable).

It's probably due to the OMXNodeInstance: !!! Observer died:

02-21 11:45:31.420 22574 22593 D scrcpy  : Controller stopped
02-21 11:45:32.469 22592 22592 I scrcpy  : Cleaning up
02-21 11:45:32.478 28310 29411 E OMXNodeInstance: !!! Observer died. Quickly, do something, ... anything...
02-21 11:45:32.590 28310 29411 W GraphicBufferSource: Dropped back down to Loaded without Executing
02-21 11:45:32.624 28310 29411 I OMX-VENC: Component Deinit

(I don't have this problem on my OnePlus 7 Pro)

Hmm, I am not familiar with OMXNodeInstance: !!! Observer died.

BTW, I don't notice this problem in my note10+ with android 11.

diff --git a/server/src/main/java/com/genymobile/scrcpy/CleanUp.java b/server/src/main/java/com/genymobile/scrcpy/CleanUp.java
index 7dda83fb..26012409 100644
--- a/server/src/main/java/com/genymobile/scrcpy/CleanUp.java
+++ b/server/src/main/java/com/genymobile/scrcpy/CleanUp.java
@@ -84,16 +84,14 @@ public final class CleanUp {
             }
         }
 
-        if (restoreNormalPowerMode) {
-            Ln.i("Restoring normal power mode");
-            if (Device.isScreenOn()) {
+        if (Device.isScreenOn()) {
+            if (powerOffScreen) {
+                Ln.i("Power off screen");
+                Device.powerOffScreen(displayId);
+            } else if (restoreNormalPowerMode) {
+                Ln.i("Restoring normal power mode");
                 Device.setScreenPowerMode(Device.POWER_MODE_NORMAL);
             }
         }
-
-        if (powerOffScreen) {
-            Ln.i("Power off screen");
-            Device.powerOffScreen(displayId);
-        }
     }
 }

I will apply this change as it is more correct.

Thank you for your test and survey.

Signed-off-by: Yu-Chen Lin <[email protected]>
@rom1v
Copy link
Collaborator

rom1v commented Mar 16, 2021

Thank you 👍 Merged into dev

@rom1v rom1v closed this Mar 16, 2021
rom1v pushed a commit that referenced this pull request Mar 16, 2021
PR #824 <#824>

Signed-off-by: Yu-Chen Lin <[email protected]>
Signed-off-by: Romain Vimont <[email protected]>
rom1v pushed a commit that referenced this pull request Mar 16, 2021
PR #824 <#824>

Signed-off-by: Yu-Chen Lin <[email protected]>
Signed-off-by: Romain Vimont <[email protected]>
rom1v added a commit that referenced this pull request Apr 30, 2021
This avoids to pass each option as individual parameter and parse them
manually (it's still "manual" in the Parcelable implementation).

Refs #824 <#824 (comment)>
rom1v added a commit that referenced this pull request May 1, 2021
This avoids to pass each option as individual parameter and parse them
manually (it's still "manual" in the Parcelable implementation).

Refs #824 <#824 (comment)>

Reviewed-by: Yu-Chen Lin <[email protected]>
rom1v added a commit that referenced this pull request Jun 11, 2021
This avoids to pass each option as individual parameter and parse them
manually (it's still "manual" in the Parcelable implementation).

Refs #824 <#824 (comment)>

Reviewed-by: Yu-Chen Lin <[email protected]>
@rom1v rom1v mentioned this pull request Oct 27, 2021
rom1v added a commit that referenced this pull request Nov 7, 2021
The option was not documented.

Refs #824 <#824>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Power off screen on close
2 participants